-
-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Colection required amount #6849
Conversation
✅ Deploy Preview for koda-canary ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
SUCCESS @pbkompasz PR for issue #6738 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime! |
AI-Generated Summary: This pull request includes five patches focused on several aspects of the code. Firstly, constants related to collection URLs and deposit amounts are added to a constants file. In the second patch, a variant attribute was added to the SubmitButton component, which allows this component to received a variant prop that modifies its appearance. Thirdly, strings relating to insufficient funds and deposit requirements have been added to the locale file to improve the application's internationalization. Fourthly, the message displayed when creating a collection was adjusted to provide more information about balance requirements and deposits. Finally, an unused import reference was removed from a collection creation component to improve the code's cleanliness and efficiency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this only available for rmrk Create collection? and it link towards AssetHub (Statemine) doc?
<a | ||
v-safe-href=" | ||
'https://hello.kodadot.xyz/multi-chain/fees/assethub-fees#polkadot-asset-hub-fees-prev.-statemint' | ||
" | ||
>Learn more</a | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<a | |
v-safe-href=" | |
'https://hello.kodadot.xyz/multi-chain/fees/assethub-fees#polkadot-asset-hub-fees-prev.-statemint' | |
" | |
>Learn more</a | |
> | |
<a | |
v-safe-href=" | |
https://hello.kodadot.xyz/multi-chain/fees/assethub-fees#polkadot-asset-hub-fees-prev.-statemint | |
" | |
>Learn more</a | |
> |
missing translation helper.learnMore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where should the 'Learn more' link to for /bsx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on the chain (urlPrefix
) you're on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
urlPrefix
has the following type Prefix = 'bsx' | 'glmr' | 'rmrk' | 'movr' | 'ksm' | 'snek' | 'ahk' | 'dot' | 'ahp';
Each one has a different deposit value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each one has a different deposit value?
Yes, maybe bsx & snek has the same, I think it can be fetched through polkadot api package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values should be fetched from the API or kept in the constants.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I use this?
export function getclassDeposit(api: ApiPromise): bigint { |
locales/en.json
Outdated
@@ -35,6 +35,7 @@ | |||
"facts": "Facts", | |||
"computed id": "Computed id", | |||
"create collection": "Create Collection", | |||
"not enough funds": "Not Enough Funds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate of confirmPurchase.notEnoughFuns
a { | ||
@include ktheme() { | ||
color: theme('k-blue') !important; | ||
} | ||
text-decoration: underline; | ||
white-space: nowrap; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... even on scoped component this can be smelly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is styling directly the a
tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes there could be side effects If we'll use another a tag on this component
components/base/SubmitButton.vue
Outdated
|
||
export interface Props { | ||
disabled?: boolean | ||
expanded?: boolean | ||
disabledIcon?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't you just check if there is icon
prop behing passed?
something like:
const hasIcon = computed(() => +props.icon)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't work because it has a default value (paper-plane
). Should I remove the default value, but that might break functionality on other pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'll keep this icon on all submit button in app, might be time to remove it.
? 'https://hello.kodadot.xyz/multi-chain/fees/assethub-fees#polkadot-asset-hub-fees-prev.-statemint' | ||
: 'https://hello.kodadot.xyz/multi-chain/fees/assethub-fees#polkadot-asset-hub-fees-prev.-statemint?' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on previous comments I was under the impression that there should be a different link based on urlPrefix
.
Is that correct? Can you also confirm what the links should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link is for Ahk
& Ahb
prefix
for the other one it looks like the page havn't been written, maybe @yangwao can help?
const token = urlPrefix.value.toUpperCase() | ||
const requiredAmount = | ||
token === 'KSM' ? COLLECTION_DEPOSIT_KSM : COLLECTION_DEPOSIT_BSX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check out assets page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the one that you've set ie https://beta.kodadot.xyz/bsx/assets/
|
Code Climate has analyzed commit 12f1a84 and detected 0 issues on this pull request. View more on Code Climate. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
I think these things should be included in:
|
how's this one? both assets hubs still missing the required amount message, right? |
Does this fix this one too? |
Closing this one because no response and PR above prob solves it, can be reopened later if needed. |
Thank you for your contribution to the KodaDot - One Stop Shop for Polkadot NFTs.
👇 __ Let's make a quick check before the contribution.
PR Type
Context
Before submitting pull request, please make sure:
Optional
Did your issue had any of the "$" label on it?
Community participation
Screenshot 📸
Copilot Summary
🤖 Generated by Copilot at 5156e82
This pull request adds a deposit feature for creating collections and enhances the user interface and feedback for this process. It introduces a
variant
prop for theSubmitButton
component, uses theNeoField
andNeoButtonVariant
components from the@kodadot1/brick
package, and updates thelocales/en.json
andutils/constants.ts
files.🤖 Generated by Copilot at 5156e82